Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WCM-3: add include logic to webhooks #862

Merged
merged 8 commits into from
Oct 2, 2024
Merged

WCM-3: add include logic to webhooks #862

merged 8 commits into from
Oct 2, 2024

Conversation

Sinnaj94
Copy link
Contributor

@Sinnaj94 Sinnaj94 commented Sep 27, 2024

Es wurde ein Webhook für E-Papers hinzugefügt. Um die Konfiguration zu vereinfachen, wurde neben der vorhandenen <exclude>-Logik zum Ausschließen von Artikeln eine <include>-Logik hinzugefügt. Bei der Implementierung wird zunächst auf die <include> und danach auf die <exclude>-Logik geschaut.

Ticket: WCM-3

Checklist

gif

@@ -43,6 +43,7 @@ def notify_after_checkin(context, event):
@grok.subscribe(zeit.cms.interfaces.ICMSContent, zeit.cms.workflow.interfaces.IPublishedEvent)
def notify_after_publish(context, event):
create_webhook_job('publish', context, countdown=5)
create_webhook_job('publish-epaper', context, countdown=5)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kann eigentlich weg, da es einfach mehrere <webhook> mit der gleichen ID aber anderer url

@Sinnaj94 Sinnaj94 marked this pull request as ready for review September 30, 2024 07:51
@Sinnaj94
Copy link
Contributor Author

Sinnaj94 commented Sep 30, 2024

Muss wohl nochmal an die Tests.
--> Habe die Logik im letzten Commit nochmal angepasst. Ich denke so sollte es passen. Was hältst du von der Lösung, Louisa? 🙂

Copy link
Contributor

@louika louika left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment 🍪

@Sinnaj94 Sinnaj94 changed the title WCM-3: add e-paper webhook WCM-3: add include logic to webhooks Sep 30, 2024
@Sinnaj94 Sinnaj94 requested a review from louika October 1, 2024 09:21
Copy link
Contributor

@louika louika left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

und noch ne Runde 😵

<exclude>
<type>testcontenttype</type>
</exclude>
</webhook>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dies soll der Fall exclude gewinnt gegen include sein, so wie in Zeile 244ff, richtig? Das ist sehr implizit. Ich schlage vor, einmal einen Test test_exclude_takes_precedence_over_include hinzuzufügen. Es gibt bisher auch keinen simplen <include> Test, analog zu den Tests, die in der WebhookExcludeTest Klasse drin sind.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

benötigen wir diesen Erweiterung der Config noch, nachdem wir die WebhookExcludeStrongerThanIncludeTest oben haben? Der Context hier sind ja die Events, ob irgendwas wegen der In-/Exclude-Bedingung rausfliegt, ist nebensächlich.

core/src/zeit/cms/checkout/webhook.py Show resolved Hide resolved
Copy link
Contributor

@louika louika left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noch eine kleine Sache 🙏 dann können wirs gern mergen
yay

<exclude>
<type>testcontenttype</type>
</exclude>
</webhook>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

benötigen wir diesen Erweiterung der Config noch, nachdem wir die WebhookExcludeStrongerThanIncludeTest oben haben? Der Context hier sind ja die Events, ob irgendwas wegen der In-/Exclude-Bedingung rausfliegt, ist nebensächlich.

@louika louika merged commit 051698a into main Oct 2, 2024
4 checks passed
@louika louika deleted the WCM-3 branch October 2, 2024 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants